Support ref #3129

hengyunabc · 2026-01-09T12:58:30Z

No description provided.

Copilot

Pull request overview

This pull request adds support for a global object reference store (#ref) feature in Arthas OGNL expressions. The feature allows users to share object references across multiple commands using weak references with namespace isolation.

Key changes:

Implemented ObjectRefStore class with weak reference storage, LRU eviction, and namespace support
Integrated #ref variable into OGNL expression context via ExpressFactory
Added comprehensive documentation in both English and Chinese for vmtool, ognl, and advice-class commands

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
core/src/main/java/com/taobao/arthas/core/command/express/ObjectRefStore.java	New implementation of weak reference storage with namespace isolation and LRU eviction
core/src/test/java/com/taobao/arthas/core/command/express/ObjectRefStoreTest.java	Basic unit tests for put/get/remove operations and namespace isolation
core/src/main/java/com/taobao/arthas/core/command/express/ExpressFactory.java	Integration to bind #ref variable into OGNL context
core/src/main/java/com/taobao/arthas/core/command/Constants.java	Updated help text to document #ref variable
site/docs/en/doc/vmtool.md	English documentation with examples of using #ref in vmtool
site/docs/en/doc/ognl.md	English documentation with detailed #ref feature explanation
site/docs/en/doc/advice-class.md	English documentation listing #ref as extra context variable
site/docs/doc/vmtool.md	Chinese documentation with examples of using #ref in vmtool
site/docs/doc/ognl.md	Chinese documentation with detailed #ref feature explanation
site/docs/doc/advice-class.md	Chinese documentation listing #ref as extra context variable

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-09T13:04:17Z

core/src/main/java/com/taobao/arthas/core/command/express/ObjectRefStore.java

+            entries.remove(storeKey);
+            return null;
+        }
+        entry.lastAccessTime = System.currentTimeMillis();


The lastAccessTime field is being updated without synchronization on the entry object itself. While the method is synchronized, the field is declared as volatile, which only guarantees visibility but not atomicity for non-atomic operations. Multiple threads could read the entry at the same time (through the synchronized method) and then update lastAccessTime, potentially causing lost updates. Consider using AtomicLong for lastAccessTime or ensure proper synchronization.

Suggested change

entry.lastAccessTime = System.currentTimeMillis();

synchronized (entry) {

entry.lastAccessTime = System.currentTimeMillis();

}

Copilot · 2026-01-09T13:04:17Z

core/src/main/java/com/taobao/arthas/core/command/express/ObjectRefStore.java

+    private void evictIfNecessary() {
+        while (entries.size() > maxEntries) {
+            Iterator<Map.Entry<StoreKey, Entry>> it = entries.entrySet().iterator();
+            if (!it.hasNext()) {
+                return;
+            }
+            it.next();
+            it.remove();
+        }
+    }


The evictIfNecessary method is not synchronized, but it modifies the shared entries map. This method is called from the put method which is synchronized, so the synchronization is inherited. However, this is not obvious and could lead to issues if this method is called from elsewhere in the future. Consider adding the synchronized modifier to make the thread-safety explicit and prevent future bugs.

Copilot · 2026-01-09T13:04:17Z

core/src/test/java/com/taobao/arthas/core/command/express/ObjectRefStoreTest.java

+package com.taobao.arthas.core.command.express;
+
+import java.util.List;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ObjectRefStoreTest {
+
+    @Test
+    public void testPutGetRemoveWithNamespace() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object value = new Object();
+
+        Express express1 = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express1.bind("v", value).bind(new Object());
+        Object putResult = express1.get("#ref.ns(\"ns1\").put(\"k\", #v)");
+        Assert.assertSame(value, putResult);
+
+        Express express2 = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express2.bind(new Object());
+        Object getResult = express2.get("#ref.ns(\"ns1\").get(\"k\")");
+        Assert.assertSame(value, getResult);
+
+        Object removeResult = express2.get("#ref.ns(\"ns1\").remove(\"k\")");
+        Assert.assertSame(value, removeResult);
+
+        Object afterRemove = express2.get("#ref.ns(\"ns1\").get(\"k\")");
+        Assert.assertNull(afterRemove);
+    }
+
+    @Test
+    public void testNamespaceIsolation() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object valueA = new Object();
+        Object valueB = new Object();
+
+        Express express = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express.bind("a", valueA).bind("b", valueB).bind(new Object());
+
+        Assert.assertSame(valueA, express.get("#ref.ns(\"a\").put(\"k\", #a)"));
+        Assert.assertSame(valueB, express.get("#ref.ns(\"b\").put(\"k\", #b)"));
+
+        Assert.assertSame(valueA, express.get("#ref.ns(\"a\").get(\"k\")"));
+        Assert.assertSame(valueB, express.get("#ref.ns(\"b\").get(\"k\")"));
+        Assert.assertNull(express.get("#ref.ns(\"c\").get(\"k\")"));
+    }
+
+    @Test
+    public void testLsAndNamespaces() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object value = new Object();
+
+        Express express = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express.bind("v", value).bind(new Object());
+        express.get("#ref.ns(\"ns-list\").put(\"k\", #v)");
+
+        List<?> ls = (List<?>) express.get("#ref.ns(\"ns-list\").ls()");
+        Assert.assertEquals(1, ls.size());
+
+        Map<?, ?> item = (Map<?, ?>) ls.get(0);
+        Assert.assertEquals("ns-list", item.get("namespace"));
+        Assert.assertEquals("k", item.get("name"));
+
+        List<?> namespaces = (List<?>) express.get("#ref.namespaces()");
+        Assert.assertTrue(namespaces.contains("ns-list"));
+    }
+}


The test lacks verification of weak reference behavior - specifically testing that objects can be garbage collected and that get() returns null after GC. This is a key feature mentioned in the documentation. Consider adding a test that explicitly triggers garbage collection and verifies that the weak reference no longer holds the object.

Copilot · 2026-01-09T13:04:18Z

core/src/test/java/com/taobao/arthas/core/command/express/ObjectRefStoreTest.java

+package com.taobao.arthas.core.command.express;
+
+import java.util.List;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ObjectRefStoreTest {
+
+    @Test
+    public void testPutGetRemoveWithNamespace() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object value = new Object();
+
+        Express express1 = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express1.bind("v", value).bind(new Object());
+        Object putResult = express1.get("#ref.ns(\"ns1\").put(\"k\", #v)");
+        Assert.assertSame(value, putResult);
+
+        Express express2 = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express2.bind(new Object());
+        Object getResult = express2.get("#ref.ns(\"ns1\").get(\"k\")");
+        Assert.assertSame(value, getResult);
+
+        Object removeResult = express2.get("#ref.ns(\"ns1\").remove(\"k\")");
+        Assert.assertSame(value, removeResult);
+
+        Object afterRemove = express2.get("#ref.ns(\"ns1\").get(\"k\")");
+        Assert.assertNull(afterRemove);
+    }
+
+    @Test
+    public void testNamespaceIsolation() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object valueA = new Object();
+        Object valueB = new Object();
+
+        Express express = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express.bind("a", valueA).bind("b", valueB).bind(new Object());
+
+        Assert.assertSame(valueA, express.get("#ref.ns(\"a\").put(\"k\", #a)"));
+        Assert.assertSame(valueB, express.get("#ref.ns(\"b\").put(\"k\", #b)"));
+
+        Assert.assertSame(valueA, express.get("#ref.ns(\"a\").get(\"k\")"));
+        Assert.assertSame(valueB, express.get("#ref.ns(\"b\").get(\"k\")"));
+        Assert.assertNull(express.get("#ref.ns(\"c\").get(\"k\")"));
+    }
+
+    @Test
+    public void testLsAndNamespaces() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object value = new Object();
+
+        Express express = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express.bind("v", value).bind(new Object());
+        express.get("#ref.ns(\"ns-list\").put(\"k\", #v)");
+
+        List<?> ls = (List<?>) express.get("#ref.ns(\"ns-list\").ls()");
+        Assert.assertEquals(1, ls.size());
+
+        Map<?, ?> item = (Map<?, ?>) ls.get(0);
+        Assert.assertEquals("ns-list", item.get("namespace"));
+        Assert.assertEquals("k", item.get("name"));
+
+        List<?> namespaces = (List<?>) express.get("#ref.namespaces()");
+        Assert.assertTrue(namespaces.contains("ns-list"));
+    }
+}


The test lacks coverage for the LRU eviction behavior when maxEntries limit is exceeded. This is an important feature mentioned in the documentation. Consider adding a test that creates more than 1024 entries and verifies that least recently used entries are evicted.

Copilot · 2026-01-09T13:04:18Z

core/src/test/java/com/taobao/arthas/core/command/express/ObjectRefStoreTest.java

+package com.taobao.arthas.core.command.express;
+
+import java.util.List;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ObjectRefStoreTest {
+
+    @Test
+    public void testPutGetRemoveWithNamespace() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object value = new Object();
+
+        Express express1 = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express1.bind("v", value).bind(new Object());
+        Object putResult = express1.get("#ref.ns(\"ns1\").put(\"k\", #v)");
+        Assert.assertSame(value, putResult);
+
+        Express express2 = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express2.bind(new Object());
+        Object getResult = express2.get("#ref.ns(\"ns1\").get(\"k\")");
+        Assert.assertSame(value, getResult);
+
+        Object removeResult = express2.get("#ref.ns(\"ns1\").remove(\"k\")");
+        Assert.assertSame(value, removeResult);
+
+        Object afterRemove = express2.get("#ref.ns(\"ns1\").get(\"k\")");
+        Assert.assertNull(afterRemove);
+    }
+
+    @Test
+    public void testNamespaceIsolation() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object valueA = new Object();
+        Object valueB = new Object();
+
+        Express express = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express.bind("a", valueA).bind("b", valueB).bind(new Object());
+
+        Assert.assertSame(valueA, express.get("#ref.ns(\"a\").put(\"k\", #a)"));
+        Assert.assertSame(valueB, express.get("#ref.ns(\"b\").put(\"k\", #b)"));
+
+        Assert.assertSame(valueA, express.get("#ref.ns(\"a\").get(\"k\")"));
+        Assert.assertSame(valueB, express.get("#ref.ns(\"b\").get(\"k\")"));
+        Assert.assertNull(express.get("#ref.ns(\"c\").get(\"k\")"));
+    }
+
+    @Test
+    public void testLsAndNamespaces() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object value = new Object();
+
+        Express express = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express.bind("v", value).bind(new Object());
+        express.get("#ref.ns(\"ns-list\").put(\"k\", #v)");
+
+        List<?> ls = (List<?>) express.get("#ref.ns(\"ns-list\").ls()");
+        Assert.assertEquals(1, ls.size());
+
+        Map<?, ?> item = (Map<?, ?>) ls.get(0);
+        Assert.assertEquals("ns-list", item.get("namespace"));
+        Assert.assertEquals("k", item.get("name"));
+
+        List<?> namespaces = (List<?>) express.get("#ref.namespaces()");
+        Assert.assertTrue(namespaces.contains("ns-list"));
+    }
+}


The test lacks coverage for the clear() method functionality. While clearAll() is tested implicitly by calling it at the beginning of each test, the clear() method that clears only a specific namespace should be explicitly tested to ensure it properly isolates namespace cleanup.

Copilot · 2026-01-09T13:04:18Z

core/src/test/java/com/taobao/arthas/core/command/express/ObjectRefStoreTest.java

+package com.taobao.arthas.core.command.express;
+
+import java.util.List;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ObjectRefStoreTest {
+
+    @Test
+    public void testPutGetRemoveWithNamespace() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object value = new Object();
+
+        Express express1 = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express1.bind("v", value).bind(new Object());
+        Object putResult = express1.get("#ref.ns(\"ns1\").put(\"k\", #v)");
+        Assert.assertSame(value, putResult);
+
+        Express express2 = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express2.bind(new Object());
+        Object getResult = express2.get("#ref.ns(\"ns1\").get(\"k\")");
+        Assert.assertSame(value, getResult);
+
+        Object removeResult = express2.get("#ref.ns(\"ns1\").remove(\"k\")");
+        Assert.assertSame(value, removeResult);
+
+        Object afterRemove = express2.get("#ref.ns(\"ns1\").get(\"k\")");
+        Assert.assertNull(afterRemove);
+    }
+
+    @Test
+    public void testNamespaceIsolation() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object valueA = new Object();
+        Object valueB = new Object();
+
+        Express express = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express.bind("a", valueA).bind("b", valueB).bind(new Object());
+
+        Assert.assertSame(valueA, express.get("#ref.ns(\"a\").put(\"k\", #a)"));
+        Assert.assertSame(valueB, express.get("#ref.ns(\"b\").put(\"k\", #b)"));
+
+        Assert.assertSame(valueA, express.get("#ref.ns(\"a\").get(\"k\")"));
+        Assert.assertSame(valueB, express.get("#ref.ns(\"b\").get(\"k\")"));
+        Assert.assertNull(express.get("#ref.ns(\"c\").get(\"k\")"));
+    }
+
+    @Test
+    public void testLsAndNamespaces() throws ExpressException {
+        ObjectRefStore.ref().clearAll();
+
+        Object value = new Object();
+
+        Express express = ExpressFactory.unpooledExpress(ObjectRefStoreTest.class.getClassLoader());
+        express.bind("v", value).bind(new Object());
+        express.get("#ref.ns(\"ns-list\").put(\"k\", #v)");
+
+        List<?> ls = (List<?>) express.get("#ref.ns(\"ns-list\").ls()");
+        Assert.assertEquals(1, ls.size());
+
+        Map<?, ?> item = (Map<?, ?>) ls.get(0);
+        Assert.assertEquals("ns-list", item.get("namespace"));
+        Assert.assertEquals("k", item.get("name"));
+
+        List<?> namespaces = (List<?>) express.get("#ref.namespaces()");
+        Assert.assertTrue(namespaces.contains("ns-list"));
+    }
+}


The test lacks coverage for edge cases such as putting null values (which should trigger removal), using null/empty namespace strings, using null/empty key strings, and concurrent access scenarios. These edge cases are important for ensuring robustness.

Copilot · 2026-01-09T13:04:18Z

site/docs/en/doc/vmtool.md

 :::

 ```bash
 vmtool --action getInstances --classLoaderClass org.springframework.boot.loader.LaunchedURLClassLoader --className org.springframework.context.ApplicationContext --express'instances[0].getBeanDefinitionNames()'


There is a missing space between "--express" and the opening quote in the command. It should be "--express 'instances[0].getBeanDefinitionNames()'" with a space after "--express".

Suggested change

vmtool --action getInstances --classLoaderClass org.springframework.boot.loader.LaunchedURLClassLoader --className org.springframework.context.ApplicationContext --express'instances[0].getBeanDefinitionNames()'

vmtool --action getInstances --classLoaderClass org.springframework.boot.loader.LaunchedURLClassLoader --className org.springframework.context.ApplicationContext --express 'instances[0].getBeanDefinitionNames()'

hengyunabc and others added 2 commits January 7, 2026 00:15

support ref part 1

729bf70

Prettified Code!

2dd9c6d

hengyunabc requested a review from Copilot January 9, 2026 12:58

Copilot started reviewing on behalf of hengyunabc January 9, 2026 12:59 View session

Copilot AI reviewed Jan 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support ref #3129

Support ref #3129

hengyunabc commented Jan 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 9, 2026

Uh oh!

Copilot AI Jan 9, 2026

Uh oh!

Copilot AI Jan 9, 2026

Uh oh!

Copilot AI Jan 9, 2026

Uh oh!

Copilot AI Jan 9, 2026

Uh oh!

Copilot AI Jan 9, 2026

Uh oh!

Copilot AI Jan 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	vmtool --action getInstances --classLoaderClass org.springframework.boot.loader.LaunchedURLClassLoader --className org.springframework.context.ApplicationContext --express'instances[0].getBeanDefinitionNames()'
	vmtool --action getInstances --classLoaderClass org.springframework.boot.loader.LaunchedURLClassLoader --className org.springframework.context.ApplicationContext --express 'instances[0].getBeanDefinitionNames()'

Support ref #3129

Are you sure you want to change the base?

Support ref #3129

Conversation

hengyunabc commented Jan 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants